Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #serializeBoxes function not output correct quadPoints values for deserialize (issue19056) #19066

Merged
merged 1 commit into from
Nov 29, 2024
Merged

Fix #serializeBoxes function not output correct quadPoints values for deserialize (issue19056) #19066

merged 1 commit into from
Nov 29, 2024

Conversation

jarklee
Copy link
Contributor

@jarklee jarklee commented Nov 18, 2024

This PR fixes the #serializeBoxes in the highlight editor to output the correct quadPoints sequence used by the deserialize method after the commit a62ceed introduce a conflict logic between two methods.

I also added the integration test to ensure both methods (#serializeBoxes and deserialize) always output the correct values for the serializing/deserializing process.

Reproduce step

// Some code to create highlight annotation

const { map } = PDFViewerApplication.pdfDocument.annotationStorage.serializable;
const vals = Array.from(map.values());
const serialized = vals[0]; // Get first serialized highlight annotation

const layer =  PDFViewerApplication.pdfViewer.getPageView(0).annotationEditorLayer.annotationEditorLayer;
const editor = await layer.deserialize(serialized);
layer.add(editor);

// New deserialized highlight will not shown.
// If you check the #boxes field, the height value of the boxes gets a negative value.
// And all y value of boxes are not correct

Snuffleupagus
Snuffleupagus reviewed Nov 18, 2024
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll leave the actual review for @calixteman, but there's a few things that you'll need to address before that.

test/integration/highlight_editor_spec.mjs Outdated Show resolved Hide resolved
test/integration/test_utils.mjs Outdated Show resolved Hide resolved
test/integration/test_utils.mjs Outdated Show resolved Hide resolved
test/integration/test_utils.mjs Outdated Show resolved Hide resolved
test/integration/test_utils.mjs Outdated Show resolved Hide resolved
@jarklee jarklee force-pushed the issue19056 branch 2 times, most recently from 711b4ad to 891482b Compare November 18, 2024 10:48
@Snuffleupagus Snuffleupagus removed their request for review November 18, 2024 11:11
Snuffleupagus
Snuffleupagus reviewed Nov 18, 2024
View reviewed changes
test/integration/highlight_editor_spec.mjs Outdated Show resolved Hide resolved
test/integration/test_utils.mjs Outdated Show resolved Hide resolved
test/integration/test_utils.mjs Outdated Show resolved Hide resolved
@Snuffleupagus Snuffleupagus removed their request for review November 18, 2024 13:13
@jarklee jarklee requested a review from calixteman November 19, 2024 01:02
@jarklee jarklee force-pushed the issue19056 branch 3 times, most recently from d991d2c to 92906b1 Compare November 20, 2024 17:26
@jarklee jarklee requested a review from calixteman November 20, 2024 17:27
@calixteman
Copy link
Contributor

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/675b389b789cd98/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/5c85ff11302748c/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/5c85ff11302748c/output.txt

Total script time: 8.89 mins

  • Integration Tests: FAILED

@calixteman
Copy link
Contributor

@jarklee did you run the tests locally ?
You can run them with npx gulp integrationtest and in changing describe to fdescribe to focus on the specific tests you want to run.

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/675b389b789cd98/output.txt

Total script time: 20.19 mins

  • Integration Tests: FAILED

@jarklee
Fix #serializeBoxes bug inconsistent with deserialize function for hi…
2200f05 
…ghlight editor.

Add test for quadPoints order while serializing.
@Snuffleupagus Snuffleupagus linked an issue Nov 27, 2024 that may be closed by this pull request
Incorrect calculation in deserialization of the highlight annotation #19056
Closed
@calixteman
Copy link
Contributor

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.193.163.58:8877/e507e0213c838ae/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/5b61ec8511ed724/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/5b61ec8511ed724/output.txt

Total script time: 8.87 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/e507e0213c838ae/output.txt

Total script time: 20.30 mins

  • Integration Tests: FAILED

calixteman
calixteman approved these changes Nov 29, 2024
View reviewed changes
Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you.

@calixteman calixteman merged commit 9ecc613 into mozilla:master Nov 29, 2024
9 checks passed
@calixteman calixteman mentioned this pull request Dec 3, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Snuffleupagus Snuffleupagus Snuffleupagus left review comments

@calixteman calixteman calixteman approved these changes

Assignees
No one assigned
Labels
editor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect calculation in deserialization of the highlight annotation
4 participants
@jarklee @calixteman @moz-tools-bot @Snuffleupagus